Fixes an issue related to prefix shared alternatives with nesting restrictions #1919
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1919 +/- ##
=======================================
Coverage 49% 49%
- Complexity 6164 6166 +2
=======================================
Files 661 661
Lines 58702 58701 -1
Branches 8547 8548 +1
=======================================
+ Hits 28962 28965 +3
Misses 27550 27550
+ Partials 2190 2186 -4 ☔ View full report in Codecov by Sentry. |
|
Thanks @arnoldlankamp ! I was "under water" the last weeks with too many distractions to look at your fixes. Will have a look now. The fix sounds logical. |
|
|
||
| public void testDoubleRightNullable() { |
There was a problem hiding this comment.
@arnoldlankamp why is this test not useful anymore?
There was a problem hiding this comment.
It's still there. This is just a tabs vs spaces formatting diff.
There was a problem hiding this comment.
smart and elegant fix. not binary compatible, so let's watch out for that when releasing the new version. Thanks @arnoldlankamp
|
I'm merging this because we have to move on, but we should realize that a careful bootstrap sequence is required if we want to avoid class loading errors of previously generated parsers. It may be necessary to also include a compatibility mode. |
| private final IntegerMap resultStoreMappings; | ||
|
|
||
| private final SortedIntegerObjectList<DoubleArrayList<P, AbstractStackNode<P>[]>> alternatives; | ||
|
|
||
| public ExpectBuilder(IntegerMap resultStoreMappings){ | ||
| public ExpectBuilder(IntegerKeyedHashMap<IntegerList> dontNest, IntegerMap resultStoreMappings){ |
There was a problem hiding this comment.
Propose to include the old constructor as well @deprecated, such that we can bootstrap without too many issues.
There was a problem hiding this comment.
Adding a constructor with the old signature, which uses an empty dontNest map, would make the change backwards compatible and should make everything work as before.
I considered this myself, but ultimately decided against it, since it (naturally) also retains the bug. Making this a breaking change forces people to regenerate any existing parsers and prevents them from continuing to use a deprecated/buggy version.
But feel free to add one if this is preferred.
Fix for #1915 .
Changes:
S::= AB | ABC, bothA's will be shared, butBwill only be shared in case no nesting restrictions are associated with theABalternative.Theoretically the
B's could still be shared, but this would involve adding even more special cases to an already fairly complicated piece of code, so I opted not to do this, since any potential gain would be minimal.NOTE: This change is not backward compatible with any previously generated parsers. If any exist outside this project, they will need to be regenerated.